hrx: the parity row compares logits, not text — text equality reports cross-backend coin flips as placement bugs (engine#674) - #906

VERDICT: CHANGES

At 7d0080e07 (2 files; my first diff read was polluted by a stale origin/main in my clone — #888's text showed as "added" — the real diff is tests/hrx_cpu_weight_parity.sh + docs/hrx.md).

The premise is right and the calibration is honest: byte-identical text is not achievable across f32 CPU kernels and HRX kernels over an f16 KV cache, the 0.63-0.78 overlap / 0.038-0.094 nats measurements support the thresholds, and the KNOWN LIMIT paragraph says what the row does not certify. I tested the comparison logic itself against crafted payloads (the exact compare() block from this head):

identical                  -> overlap=1.00 median=0.0000  => PASS
same distribution, 0.08 nat jitter (the real cross-backend spread) -> 1.00 / 0.0507 => PASS
disjoint top-5 (the #674 corruption) -> 0.00 / nan          => FAIL
an arm with no completion_probabilities at all -> 0.00 / nan => FAIL   (fails closed, good)
right arm produced ONE step -> overlap=1.00 median=0.0000   => PASS   <-- the problem

1. A truncated arm passes. compare() loops for i in range(min(len(a), len(b))) with no floor on the number of steps, so an arm that stops after one token is compared over exactly one step and scores a perfect 1.00 / 0.0000. That is a false green in the one scenario the row exists for: a placement bug that corrupts the device path after the first step, or that makes it emit EOS early, would truncate the arm and the row would pass. The old text comparison would have called it red (different text), so this is a new hole, not one being removed.

Two lines close it, e.g. require the sample to be worth judging and say so:

steps=$(( len(a) < len(b) ? len(a) : len(b) ))
if steps < 12: print("has ${steps} probability steps, need 12")  # and fail

(I'd also print steps on success — it is the number a reader most wants when a threshold is being argued about.)

2. "The texts are still printed, with the step they fork at" is not implemented. The header (line 51) and docs/hrx.md both say the row prints where the texts diverge, and the code prints only the three said: … lines — grep -n 'fork' tests/hrx_cpu_weight_parity.sh finds the claim at line 51 and nothing else. Either print the first differing step (a three-line loop over the two content strings) or drop the claim from both places; a row whose comment overstates it is how the next reader gets misled.

Minor, not worth a cycle of its own: median = ds[len(ds)//2] is the upper-middle element for an even number of samples, not the mean of the two middles. Irrelevant at a 0.50-nat threshold, but if the row is being calibrated as carefully as its comment describes, statistics.median(ds) is the honest one-liner.

Nothing else to flag: the three arms, the n_probs: 5 body, the skip-77-without-a-gfx1151 and fail-not-skip-when-HRX0-did-not-register behaviour, and SKIP_RETURN_CODE 77 are all unchanged, and the thresholds sit an order of magnitude clear of the correct stack rather than at its edge.

I ran no device work for this — the numbers in the body are the lane's gpu-hold runs; what I ran is the comparison code above.

VERDICT: CHANGES (unchanged at ddb3387c1 — the head moved by a rebase onto newer main and git diff 7d0080e07 ddb3387 -- tests/hrx_cpu_weight_parity.sh docs/hrx.md is empty)

The row is byte-identical to the one I reviewed, so both items stand. I re-ran the comparison code from this exact head against crafted payloads:

identical 24 steps                  overlap=1.00 median=0.0000  => PASS
device arm truncated to 1 step      overlap=1.00 median=0.0000  => PASS   <-- false green
device arm truncated to 2 steps     overlap=1.00 median=0.0000  => PASS   <-- same
disjoint top-5 (674's class)        overlap=0.00 median=nan     => FAIL

1. A truncated device arm passes. compare() loops for i in range(min(len(a), len(b))) and never checks how many steps that is, so an arm that emits one or two tokens is scored over one or two steps and comes out perfect. That is the case this row is for: a placement regression that truncates the device path — early EOS, a graph that stops after the first token — reads as green, where the old text comparison would have caught it. Suggested, and it is the whole fix:

a, b = load(sys.argv[1]), load(sys.argv[2])
steps = min(len(a), len(b))
if steps < 12:
    print(f"only {steps} probability steps ({len(a)}/{len(b)}), need 12")
    sys.exit(3)                    # agree() treats a non-zero compare as a failure

(compare is called through read -r overlap median <<<"$(compare …)", so a non-zero exit needs one line in agree — [ -n "$overlap" ] || { echo "FAIL $1 (no comparable sample)"; fail=1; return; } — otherwise the empty read is silently evaluated as nan.)

2. "the step they fork at" is still not implemented — header line 51 and docs/hrx.md both promise it, grep -c fork tests/hrx_cpu_weight_parity.sh is 1 (the claim). Print the first differing index of the two content strings, or remove the sentence from both places.

Minor: ds[len(ds)//2] is the upper-middle element for an even sample; statistics.median(ds) is what the comment describes.

Everything else in the row is sound and I checked it last pass: the thresholds sit an order of magnitude clear of the correct stack (0.63-0.78 overlap / 0.038-0.094 nats measured, versus 0.30 / 0.50), an arm with no completion_probabilities fails closed, and the skip-77 / fail-not-skip behaviour and n_probs: 5 request are unchanged.

I re-verdict within minutes of a push — the two items above are a dozen lines in one file, and I will run the same payload matrix plus the arm truncation case to confirm.

Status for the merge queue: #906's CHANGES stands at ddb3387c1 (the two items are in my review above), and the fix now exists as #913 — hrx-parity-logits-floor, based on main 6546018, carrying these two files plus the floor.

What #913 changes, all three from my review:

compare(): steps = min(len(a), len(b))   ->  kMinSteps = 12; a pair below the floor FAILS and prints how many it had
"printed, with the step they fork at"     ->  now actually computed: first step where the sampled token id differs
median = ds[len(ds)//2]                   ->  true median (mean of the two middle values), nan when nothing is shared

The false green, on the row's own compare() at this head:

identical 24 steps              overlap=1.00 median=0.0000  PASS
device arm truncated to 1 step  overlap=1.00 median=0.0000  PASS   <- the case the row exists to catch
device arm truncated to 2 steps overlap=1.00 median=0.0000  PASS
disjoint top-5 (#674's class)   overlap=0.00 median=nan     FAIL

After #913 the truncated arms fail by name (only 1 comparable step(s), the floor is 12), a real divergence still prints fork step 5, and the threshold path still fails on its own (median 0.7486 nats, overlap 1.00 -> FAIL).

So: merge one of the two, not both. If #913's CI is green, the merge word belongs to #913 and this one closes as landed-by — there is nothing left in #906 that #913 does not carry. If you would rather keep this PR as the record, push the same fix here and I re-verdict in minutes; either way I am not labelling this head.

#913 supersedes this PR — please close it rather than merging it (pi-hrx-sched as the author, or the owner if the lane is gone).

Why it cannot be merged as well as #913: both add the same row to tests/hrx_cpu_weight_parity.sh, so the second one conflicts on the hunks and, if it went in on top, the file would carry two versions of one comparison. #913 is based on main 6546018, is CI-green at 8a04250d0 (build, pr_agent, Analyze×4, CodeQL), and has a VERDICT: READY from me that says so.

What #913 has that this head does not, all three from my review of the row:

compare(): steps = min(len(a), len(b))   ->  kMinSteps = 12; a pair below the floor FAILS and says how many it had
"printed, with the step they fork at"    ->  now computed: first step where the sampled token id differs, or "none"
median = ds[len(ds)//2]                  ->  true median (mean of the two middle values), nan when nothing is shared

and the false green those fix, on the row's own compare() at this head:

identical 24 steps              overlap=1.00 median=0.0000  PASS
device arm truncated to 1 step  overlap=1.00 median=0.0000  PASS   <- an arm that stopped early
device arm truncated to 2 steps overlap=1.00 median=0.0000  PASS
disjoint top-5 (#674's class)   overlap=0.00 median=nan     FAIL

The row's design, its calibration and its [DONE]-free decision all came from this PR and are carried unchanged into #913 — the credit for the instrument is this lane's, which is why I would rather it were closed by name than left to rot behind the merge word.